MON-4033: Add OpenShiftMetricsConfig#2726
MON-4033: Add OpenShiftMetricsConfig#2726danielmellado wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@danielmellado: This pull request references MON-4033 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new OpenShiftStateMetricsConfig type and field to ClusterMonitoringSpec (v1alpha1) with NodeSelector, Resources, Tolerations, and TopologySpreadConstraints. Generated deepcopy and Swagger doc functions for the new type. Updated the CRD manifest to expose openShiftStateMetricsConfig with detailed schema validation for nested fields, quantities, and array sizes. Added three onCreate test cases in ClusterMonitoringConfig.yaml covering: valid resources, valid topologySpreadConstraints, and all fields present. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
578-591: Make the duplicate topology spread assertion more specific.Line 591 uses a very broad substring (
"Duplicate value"), which can hide failures from the wrong field/path.🔧 Suggested test assertion tightening
- expectedError: "Duplicate value" + expectedError: 'spec.openShiftStateMetricsConfig.topologySpreadConstraints[1]: Duplicate value'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml` around lines 578 - 591, The test "Should reject OpenShiftStateMetricsConfig with duplicate topologySpreadConstraints" is asserting a too-generic error ("Duplicate value"); update the expectedError to assert the precise validation message and path (for example include spec.openShiftStateMetricsConfig.topologySpreadConstraints and the duplicate topologyKey/value) so the test verifies the exact field/path that failed (target the openShiftStateMetricsConfig.topologySpreadConstraints duplicate topologyKey error string rather than the broad "Duplicate value").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 578-591: The test "Should reject OpenShiftStateMetricsConfig with
duplicate topologySpreadConstraints" is asserting a too-generic error
("Duplicate value"); update the expectedError to assert the precise validation
message and path (for example include
spec.openShiftStateMetricsConfig.topologySpreadConstraints and the duplicate
topologyKey/value) so the test verifies the exact field/path that failed (target
the openShiftStateMetricsConfig.topologySpreadConstraints duplicate topologyKey
error string rather than the broad "Duplicate value").
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
danielmellado
force-pushed
the
openshift-metrics-config
branch
from
8412fe0 to
bd28c7e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
642-687: Consider adding boundary tests fornodeSelectorandtolerations.You validate
resourcesandtopologySpreadConstraintsthoroughly, butnodeSelector(min/maxProperties) andtolerations(min/maxItems) constraints are not explicitly exercised yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml` around lines 642 - 687, Add explicit boundary test cases for openShiftStateMetricsConfig.nodeSelector and openShiftStateMetricsConfig.tolerations: add tests that cover nodeSelector min/maxProperties (one case with zero properties if min>0, one with exactly the max allowed properties, and one exceeding max) and tolerations min/maxItems (one with zero items if min>0, one with exactly the max allowed items, and one exceeding max). Use clear test names (e.g., "Should accept OpenShiftStateMetricsConfig with nodeSelector at maxProperties", "Should reject OpenShiftStateMetricsConfig with nodeSelector exceeding maxProperties", "Should accept OpenShiftStateMetricsConfig with tolerations at maxItems", "Should reject OpenShiftStateMetricsConfig with tolerations exceeding maxItems") and populate the initial/expected YAML for openShiftStateMetricsConfig.nodeSelector and openShiftStateMetricsConfig.tolerations accordingly so validation exercises both min/max constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 642-687: Add explicit boundary test cases for
openShiftStateMetricsConfig.nodeSelector and
openShiftStateMetricsConfig.tolerations: add tests that cover nodeSelector
min/maxProperties (one case with zero properties if min>0, one with exactly the
max allowed properties, and one exceeding max) and tolerations min/maxItems (one
with zero items if min>0, one with exactly the max allowed items, and one
exceeding max). Use clear test names (e.g., "Should accept
OpenShiftStateMetricsConfig with nodeSelector at maxProperties", "Should reject
OpenShiftStateMetricsConfig with nodeSelector exceeding maxProperties", "Should
accept OpenShiftStateMetricsConfig with tolerations at maxItems", "Should reject
OpenShiftStateMetricsConfig with tolerations exceeding maxItems") and populate
the initial/expected YAML for openShiftStateMetricsConfig.nodeSelector and
openShiftStateMetricsConfig.tolerations accordingly so validation exercises both
min/max constraints.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (3)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (5)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.goconfig/v1alpha1/zz_generated.deepcopy.goconfig/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1alpha1/zz_generated.swagger_doc_generated.go
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Changes look good to me - great work!
/lgtm
|
Scheduling tests matching the |
openshift-ci
bot
added
the
approved
|
/retest-required |
3 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@danielmellado: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
This commit adds configuration options for the openshift-state-metrics agent in config/v1alpha1 The new struct supports: - nodeSelector: node scheduling constraints - resources: compute resource requests and limits - tolerations: pod tolerations - topologySpreadConstraints: pod distribution across topology domains Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
danielmellado
force-pushed
the
openshift-metrics-config
branch
from
bd28c7e to
ccf008e
Compare
openshift-ci-robot
removed
the
verified
openshift-merge-robot
removed
the
needs-rebase
|
@everettraven I ahd to rebase this in the middle of the merge xD, mind approving it again? Thanks! |
|
@danielmellado: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
This commit adds configuration options for the openshift-state-metrics
agent in config/v1alpha1
The new struct supports:
Signed-off-by: Daniel Mellado dmellado@fedoraproject.org